Skip to content

chore(reconcile): drop legacy code#767

Merged
cbartz merged 18 commits intomainfrom
chore/drop-legacy-reconcile-ISD-4104
Mar 26, 2026
Merged

chore(reconcile): drop legacy code#767
cbartz merged 18 commits intomainfrom
chore/drop-legacy-reconcile-ISD-4104

Conversation

@cbartz
Copy link
Copy Markdown
Collaborator

@cbartz cbartz commented Mar 25, 2026

Overview

Drop all the dead code due to the move to use a pressure reconciler.

Rationale

After recent code has been merged, we no longer use reactive or legacy reconciliation (with runner_scaler). We should clean the code base to remove all redundant code.

What changed

  • Removed RunnerScaler, reactive spawning, and MongoDB integration entirely
  • Renamed NonReactive* classes → Runner* (e.g. NonReactiveConfiguration → RunnerConfiguration)
  • Dropped the n- infix for new VMs; existing VMs with n- or r- infix are still recognized via _legacy_infix on InstanceID
  • Removed reactive_runner_names output from terraform product module
  • Removed dead _log_prev_state method and SENSITIVE_PLACEHOLDER
  • Tightened expected_runners type from int | None → int
  • Added Field(ge=1) validation for reconcile_interval and charm-level validation
  • Updated docs: charm-architecture diagram, COS reference, integrations, changelog (upgrade note about removing MongoDB relation first)

Upgrade note

Operators must run juju remove-relation github-runner mongodb before upgrading, since the mongodb endpoint is removed from charmcraft.yaml.

Checklist

  • The charm style guide was applied.
  • The contributing guide was applied.
  • The changes are compliant with ISD054 - Managing Charm Complexity
  • The documentation for charmhub is updated.
  • The PR is tagged with appropriate label (urgent, trivial, complex).
  • The changelog is updated with changes that affects the users of the charm.
  • The application version number is updated in github-runner-manager/pyproject.toml.

cbartz added 7 commits March 24, 2026 21:55
Drop RunnerScaler, reconcile_service, and reactive spawning code paths.
The charm now always uses PressureReconciler (with or without a planner
relation). This removes ~3700 lines of dead code and the mongodb
relation from charmcraft.yaml.

Application changes:
- Delete runner_scaler.py, reconcile_service.py, reactive/ package
- Remove ReactiveConfiguration, QueueConfig from configuration
- Remove reactive param from InstanceID.build() and create_runners()
- Remove --python-path CLI option, always use pressure reconciler
- Bump version to 0.17.0

Charm changes:
- Remove ReactiveConfig, database param from CharmState.from_charm()
- Remove DatabaseRequires, MongoDB event handlers from charm.py
- Remove MissingMongoDBError, mongodb relation, reactive logrotate config
- Remove create_runner_scaler and _get_reactive_configuration from factories
- Remove --python-path from systemd ExecStart

Docs:
- Delete docs/how-to/reactive.md
- Update changelog with legacy removal notes
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR removes the legacy reactive reconciliation and related MongoDB integration paths now that the pressure reconciler is the only supported reconciliation mechanism, and updates configs/tests/docs accordingly across the charm and the github-runner-manager library.

Changes:

  • Remove MongoDB/reactive reconciliation code paths (including reactive modules, RunnerScaler, and reconcile service).
  • Rename/update configuration models and serialized keys (e.g., runner_configuration) and update unit/integration tests.
  • Clean up operational artifacts (logrotate config, Terraform outputs) and update docs/changelog.

Reviewed changes

Copilot reviewed 49 out of 49 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
tests/unit/test_manager_service.py Update expected rendered config section key.
tests/unit/test_logrotate.py Adjust logrotate config assertions after removal.
tests/unit/test_factories.py Update configuration model imports/fixtures for runner config.
tests/unit/test_charm_state.py Remove reactive/MongoDB state parsing tests and signature updates.
tests/unit/test_charm.py Remove MongoDB integration event handling tests.
tests/unit/factories.py Remove MongoDB integration from charm factory relations.
tests/unit/conftest.py Remove reactive config from complete charm state fixture.
terraform/product/outputs.tf Drop reactive runner Terraform output.
terraform/product/README.md Remove reactive output from generated TF docs.
src/logrotate.py Remove reactive runner logrotate config and writes.
src/grafana_dashboards/metrics.json Remove reactive-specific dashboard text.
src/factories.py Remove legacy scaler creation; use runner_configuration.
src/errors.py Remove MissingMongoDBError.
src/charm_state.py Remove MongoDB/ReactiveConfig; simplify from_charm signature.
src/charm.py Remove MongoDB relation wiring and related handlers.
github-runner-manager/tests/unit/test_runner_scaler.py Delete legacy RunnerScaler unit tests.
github-runner-manager/tests/unit/test_config.py Update config YAML schema/tests to runner_configuration.
github-runner-manager/tests/unit/reactive/test_runner_manager.py Delete reactive runner manager tests.
github-runner-manager/tests/unit/reactive/test_process_manager.py Delete reactive process manager tests.
github-runner-manager/tests/unit/reactive/test_consumer.py Delete reactive consumer tests.
github-runner-manager/tests/unit/reactive/init.py Remove reactive test package marker.
github-runner-manager/tests/unit/openstack_cloud/test_openstack_runner_manager.py Update InstanceID construction without reactive flag.
github-runner-manager/tests/unit/openstack_cloud/test_openstack_cloud.py Update InstanceID construction without reactive flag.
github-runner-manager/tests/unit/metrics/test_runner.py Update InstanceID construction in metrics tests.
github-runner-manager/tests/unit/manager/test_runner_manager.py Update create_runners call signature in tests.
github-runner-manager/tests/unit/manager/test_pressure_reconciler.py Switch to runner_configuration naming.
github-runner-manager/tests/unit/manager/test_models.py Update InstanceID tests for new naming/back-compat parsing.
github-runner-manager/tests/unit/factories/runner_instance_factory.py Remove factory field for reactive flag.
github-runner-manager/tests/integration/factories.py Update generated config to runner_configuration.
github-runner-manager/src/github_runner_manager/reconcile_service.py Remove legacy reconcile service module.
github-runner-manager/src/github_runner_manager/reactive/types_.py Remove reactive types module.
github-runner-manager/src/github_runner_manager/reactive/runner_manager.py Remove reactive reconcile logic module.
github-runner-manager/src/github_runner_manager/reactive/runner.py Remove reactive runner script.
github-runner-manager/src/github_runner_manager/reactive/process_manager.py Remove reactive process manager.
github-runner-manager/src/github_runner_manager/reactive/consumer.py Remove reactive consumer implementation.
github-runner-manager/src/github_runner_manager/reactive/init.py Remove reactive package.
github-runner-manager/src/github_runner_manager/metrics/events.py Update reconciliation event docs for non-reactive world.
github-runner-manager/src/github_runner_manager/manager/runner_scaler.py Remove legacy RunnerScaler implementation.
github-runner-manager/src/github_runner_manager/manager/runner_manager.py Remove reactive flag from create/spawn paths; update InstanceID generation.
github-runner-manager/src/github_runner_manager/manager/pressure_reconciler.py Update types/names to new runner configuration model.
github-runner-manager/src/github_runner_manager/manager/models.py Remove reactive field from InstanceID; preserve legacy infix parsing.
github-runner-manager/src/github_runner_manager/configuration/base.py Replace non-reactive/reactive config models with runner configuration.
github-runner-manager/src/github_runner_manager/configuration/init.py Export updated configuration classes.
github-runner-manager/src/github_runner_manager/cli.py Update CLI to use runner_configuration.
github-runner-manager/pyproject.toml Bump github-runner-manager version to 0.17.0.
docs/how-to/reactive.md Remove reactive how-to guide.
docs/how-to/index.rst Remove reactive how-to from navigation.
docs/changelog.md Add entry for reactive/MongoDB removal.
charmcraft.yaml Remove mongodb relation; adjust config description wording.
Comments suppressed due to low confidence (3)

src/charm_state.py:335

  • reconcile_interval validation is inconsistent: the code rejects values < 2, but the logged/raised messages say the value must be ��>= 1. Decide whether 1-minute intervals are allowed (change the check to < 1) or keep the minimum at 2 and update the messages accordingly, so users get correct guidance.
    charmcraft.yaml:164
  • max-total-virtual-machines still documents behavior tied to MongoDB/reactive mode (integrated with MongoDB, reactive mode, etc.), but this PR removes the MongoDB relation and reactive spawning. Update this option’s description to reflect the new semantics (or remove the MongoDB/reactive references) to avoid misleading operators.
      default: 0
      description: >-
        If the github-runner charm is integrated with MongoDB, the reactive mode will not spawn
        new virtual machines if there are max-total-virtual-machines of more virtual machines
        managed by the charm.

terraform/product/outputs.tf:12

  • This module drops the reactive-specific Terraform output, but terraform/charm/outputs.tf still advertises mongodb_client = "mongodb" in its requires output. That output is now stale given the MongoDB relation removal, so it should be updated to avoid consumers attempting to wire a non-existent integration.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

github-runner-manager/src/github_runner_manager/configuration/base.py:66

  • reconcile_interval is currently an unconstrained int. If a user sets it to 0, the pressure reconciler’s reconcile loop will effectively busy-spin (Event.wait(0) returns immediately), causing high CPU usage. Please enforce a safe minimum (e.g., Field(gt=0) / Field(ge=1)) and/or validate it when loading config so invalid values fail fast with a clear error.
    planner_url: Optional[AnyHttpUrl] = None
    planner_token: Optional[str] = None
    reconcile_interval: int

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 50 out of 50 changed files in this pull request and generated 4 comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated 2 comments.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

github-runner-manager/src/github_runner_manager/metrics/events.py:143

  • Reconciliation.expected_runners is still typed as int | None, but the updated docstring now states it is always present. With reactive mode removed, either tighten the field type to int (and rely on validation) or update the docstring to reflect that None is still allowed (and why).
        expected_runners: The expected number of runners.
        duration: The duration of the reconciliation in seconds.
    """

    flavor: str
    crashed_runners: int
    idle_runners: int
    active_runners: int
    expected_runners: int | None
    duration: NonNegativeFloat

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 53 out of 53 changed files in this pull request and generated no new comments.

@cbartz cbartz marked this pull request as ready for review March 25, 2026 15:16
@cbartz cbartz merged commit e58b3bf into main Mar 26, 2026
92 of 99 checks passed
@cbartz cbartz deleted the chore/drop-legacy-reconcile-ISD-4104 branch March 26, 2026 06:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants